-
Notifications
You must be signed in to change notification settings - Fork 183
RUST-1529 Use AWS SDK to get credentials #1435
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I forgot to mention this regarding the testing commit I provided, but we should leave line 226 (patchable: false
) in config.yml commented out while this PR is being worked on. patchable: false
disables tests from running on every commit to a pull request branch - normally, we don't need to run the AWS authentication tests that frequently, but since we're making AWS-related changes we should be running the tests on this PR.
let aws_credential = { | ||
// Limit scope of this variable to avoid holding onto the lock for the duration of | ||
// authenticate_stream. | ||
let cached_credential = CACHED_CREDENTIAL.lock().await; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the SDK handle credential caching? We should make sure the new implementation still caches credentials as outlined in the spec
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Following up on this — as we discussed, the SDK does implement credential caching. I’ve linked the relevant documentation here in case it's helpful for future reference!
…redential_types::Credential
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry if some of my comments are duplicated! the github UI was being weird
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! tagging Abraham for review, and then once he approves you can remove the patchable: false
change in .evergreen/config.yml
src/client/auth/aws.rs
Outdated
@@ -246,6 +300,21 @@ impl AwsCredential { | |||
} | |||
} | |||
|
|||
// Creates AwsCredential from keys. | |||
fn from_sdk_creds( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe have this take a Credentials
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do! Just as a heads-up, the code for from_sdk_creds
will be removed in my next PR when I switch to the AWS SDK for SigV4 signing, so I didn’t prioritize cleaning it up too much. That said, your suggestion is definitely cleaner! :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Replace existing implementation for getting AWS credentials by using the AWS SDK.
Currently, we have 5 places where we search for AWS credentials:
A custom AWS credential provider if the driver supports itTesting
For the sake of testing, the existing implementation was replaced instead of being hidden behind a feature flag.
Work to be done for RUST-1529
compute_authorization_header(...)
)